✨ Switch all markdown rendering to react-remark #2872
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
This was referenced Oct 30, 2023
danyx23
force-pushed
the
switch-markdown-renderer
branch
from
5ff3378 to
b1f2f96
Compare
danyx23
changed the title
danyx23
force-pushed
the
markdown-add-react-remark
branch
from
7a3fe54 to
002618e
Compare
danyx23
force-pushed
the
switch-markdown-renderer
branch
from
387b8de to
0e1f343
Compare
danyx23
force-pushed
the
markdown-add-react-remark
branch
from
002618e to
5d10eda
Compare
danyx23
force-pushed
the
switch-markdown-renderer
branch
2 times, most recently
from
3ac6675 to
e92588c
Compare
danyx23
commented
Nov 1, 2023
packages/@ourworldindata/components/src/MarkdownTextWrap/MarkdownTextWrap.test.ts
Outdated
Show resolved
Hide resolved
danyx23
requested review from
marcelgerber,
mlbrgl,
ikesau and
sophiamersmann
and removed request for
marcelgerber and
mlbrgl
marcelgerber
approved these changes
Nov 6, 2023
packages/@ourworldindata/components/src/MarkdownTextWrap/MarkdownTextWrap.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
717
to
720
| function convertMarkdownRootToIRTokens( | ||
| node: Root, | ||
| fontParams?: IRFontParams | ||
| ): IRToken[] { | ||
| return node.children.flatMap((item) => | ||
| convertMarkdownNodeToIRTokens(item, fontParams) | ||
| ) | ||
| } |
There was a problem hiding this comment.
I think it's fine to inline this into convertMarkdownToIRTokens - but you decide :)
|
Just noticed that this goes wrong on http://staging-site-switch-markdown-renderer/grapher/days-since-100th-covid-case: |
danyx23
force-pushed
the
markdown-add-react-remark
branch
from
4dc9fe3 to
6c06232
Compare
danyx23
force-pushed
the
switch-markdown-renderer
branch
from
01111b6 to
3e3744f
Compare
marcelgerber
reviewed
Nov 7, 2023
| @@ -15,6 +15,7 @@ import { TextWrap } from "../TextWrap/TextWrap.js" | |||
| import fromMarkdown from "mdast-util-from-markdown" | |||
| import type { Root, Content } from "mdast" | |||
| import { match } from "ts-pattern" | |||
| import { dropRightWhile, dropWhile } from "lodash" | |||
There was a problem hiding this comment.
And this right here will increase bundlesize quite a bit - make sure to import lodash via Util.ts always.
We should probably get rid of that annoyance at some point, but for now that's how it goes
There was a problem hiding this comment.
Really? Our bundlewatch GA didn't show any increase. I thought that importing { _ } from "lodash" was the big no-no. What am I missing?
There was a problem hiding this comment.
Ah you're right!
I was missing my own change half a year ago 😬
Merge activity
|
Also fixed a few other issues
…ownTextWrap.tsx Co-authored-by: Marcel Gerber <[email protected]>
danyx23
force-pushed
the
markdown-add-react-remark
branch
from
6c06232 to
f239a48
Compare
danyx23
force-pushed
the
switch-markdown-renderer
branch
from
531cc39 to
3863877
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR continues replacing our old custom markdown subset parser with react-remark and related functions.
We want to keep rendering markdown inside Grapher as svg text that is limited vs what markdown can offer and we have some existing svg text measuring and line break logic that we don't want to replace. To keep the changes to a relative minimum I therefore decided to translate the mdast elements to IRToken ones so that the existing logic can mostly stay in place.
Note that we are not using the latest version of mdast and related libraries as they switched to ESM only in Summer 2021 and our server admin project for example doesn't work with that yet.
The SVG tester shows quite a number of differences - I recommend reviewing the differences on the follow up branch that adds plain url parsing. Once the plain URL parsing is added, the following differences in chart subtitle/footnote rendering remain:
1) blaor- blathen this is now condensed into a single line and there is a weird backslash. There are 2 charts that do this that I saw in the svg tester but they are old covid charts so I think this is fine for now. We can reconsider if we want to allow lists/bullet points in subtitles in the future